-
Notifications
You must be signed in to change notification settings - Fork 226
Add CowStr
enum
#615
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add CowStr
enum
#615
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want the unit tests in the same PR, to ensure they don't get forgotten.
Can you please motivate a bit more the reasons for this PR (Through a benchmark for example) ? Here it looks like |
Hi @zeenix ,
Thanks for putting time into my PR. |
Hi @sgued, Thanks for putting time to my PR |
Thanks for addressing my concerns. 👍
I think you may have forgotten to push the tests cause I don't see any. 🤔 |
Hey @zeenix I have added the missing tests, sorry for the inconvenience! |
You still haven't justified the benefits for this. What is a use-case where the performance or memory usage here is noticeable? |
Thanks for the review. @sgued, I am now aware that If you are fine with it! then, I can add a Criterion benchmark comparing:
When the ‘no-change’ path dominates, this eliminates most copying. That can reduce cycle count and energy on MCUs. If the caller ultimately always needs an owned value, then I agree CowStr doesn’t help in that situation. If this conditional-clone use-case is not compelling for heapless itself, I’m OK Thanks allot for putting time into my PR :D And sorry for taking your time. |
Please provide such a benchmark. |
Complete agree with @sgued here. This needs a very compelling case. |
Hey @zeenix and @sgued thanks again for putting time to my PR! I have added a cow_str.rs in a new directory named
And looking at the terminal results it seems like in allot of places performance has regressed, many places performance has no change, and in 2 to 3 places performance has improved! I am including a snippet of the terminal output of the TL;DR: No significant change: Many benchmarks show no statistically significant difference. Improvements: A few cases improved (e.g. cowstr_creation/owned_short, cowstr_from/from_string). Terminal output:
Looking at the very few improvements and lots of regressions, I'd appreciate your guidance upon, if I should:
|
benches/cow_str.rs
Outdated
_ => unreachable!(), | ||
}; | ||
|
||
match size { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of matching, why not just split this into another function that takes size as a const generic?
@KushalMeghani1644 Thanks so much for looking into extensive benchmarking. 👍 I have to admit that I'm having a bit of difficulty understanding what exactly is being compared. I think it would be much easier to measure and see the actual benefits (if any) of this PR, if you could add the benchmarks that only use the existing API and then modified to make use of BTW, please do not open a new PR for this. Just force push to this branch. |
5c0029b
to
ea3bd5b
Compare
Hey @zeenix I have changed everything as you requested: Changes Made:
Commit 1: "Add baseline benchmarks for String operations" - Contains benchmarks using only the existing String API Commit 2: "Add CowStr clone-on-write string type" - Adds the complete CowStr implementation with tests Commit 3: "Add CowStr benchmarks comparing against String baseline" - Adds comprehensive CowStr benchmarks that can be compared against the baseline This structure makes it easy to run benchmarks before and after the CowStr
Both benchmark files compile successfully and all tests pass. The benchmarks can now be run to compare String cloning performance against CowStr's clone-on-write behavior. Thanks a lot for putting time to my PR :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for updating the PR. However, you're adding new benchmarks for the CowStr in the last commit. I asked for modifying the benchmarks to make use of the CowStr, so it's easier to see/observe the difference.
Moreover, we have a suspicion that you're using AI tools for not only the coding part but also writing your comments. While I have no objections on making use of AI tools for the code (as long as it's good quality code), I think using AI to reply to us, is unacceptable. Please ensure that your following comments on this PR are written exclusively by you.
Hey @zeenix I get your concerns regarding the use of AI tools, but just to clearify myself, I used AI ONLY for helping me make the bench! and the comments and the commit messages, all were by my own! and as you said: "While I have no objections on making use of AI tools for the code (as long as it's good quality code), I think using AI to reply to us, is unacceptable. Please ensure that your following comments on this PR are written exclusively by you." I had manually reviewed the codes AI was suggesting me before implementing, and as of the comments/messages they are indeed EXCLUSIVELY written by me. If you want instead of having an entirely new benchmark I can indeed make changes in the old existing one aswell, I had made a new bench since my local clone of repo was getting messier :) I'll appreciate your response. |
I'm sorry but I find that hard to believe, given that the wording of your comment here matches the style of Claude 100%. But ok, let's move on..
I think there is a misunderstanding. I'm not asking you to modify the existing benchmarks. You can start completely fresh if that's easier. What I am asking is how they should be organised: A commit that introduces the benchmarks w/o using CowStr and then a subsequent commit modifies those existing benchmarks to switch to CowStr use. That way, if I run the benchmarks on both commits, the output of the I hope it's clear now.
I understand but creating new benchmarks isn't a problem here. I guess you can think of this as an opportunity to learn git essentials (e.g interactive rebasing and fixups etc). Talking of which, please rebase on master, rather than merging it so we don't have redundant commits in the PR. 🙏 |
I am sorry but this is kinda harsh :) I don't even use Claude anymore I use GPT-5 for helping me for code assistance xD and as I stated, I don't use AI for commenting on whatever you ask me to do, but as you said, let's move on :) So just to clearify you need me to make a bench that doesn't uses the Thanks |
I apologise if I'm wrong.
Yes, accept we'll still want separate benchmarks for different cases. Just to clarify further, this doesn't necessarily mean you need to start the benchmarks from scratch. Do it the way its easiest for you, just as long as the end result is what is being requested. 👍 |
cc23ead
to
34a5046
Compare
Hey @zeenix I have done everything as you asked, with a clear commit history which adds a benchmark for comparing my changes before vs after ;) thanks for helping me, I am sorry to you if I was harsh on my previous comments, and if I took too long for this. |
Overview
This PR adds a
Cow<'a, N, LenT>
type to the heapless crate, enabling efficient clone-on-write strings compatible with heapless data structures.Features
Cow::Borrowed(&StringView<LenT>)
for zero-copy references.Cow::Owned(String<N, LenT>)
for owned heapless strings.to_owned()
,as_str()
,is_borrowed()
,is_owned()
.From<&StringView<LenT>>
andFrom<String<N, LenT>>
for easy construction.Borrow<str>
for compatibility with APIs expecting&str
.Motivation
Currently, heapless strings require manual cloning or reference handling.
Cow
simplifies this by providing a single type that can efficiently represent either a borrowed or owned string.Testing
Verified compilation and basic API usage. Unit tests for
Cow
will follow in a separate PR.Cow
type #582